Speedup CI #1852

AntoniosBarotsis · 2023-08-28T14:30:15Z

This Pull Request closes #1844.

Changes

Fix Warnings

use actions/checkout@v3
switch from actions-rs/toolchain (not maintained) to dtolnay/rust-toolchain
switch from actions-rs/audit-check (not maintained) to rustsec/audit-check

These changes resolved all the warnings.

Caching

Used rust-cache as mentioned in the issue.

Note
rust-cache:

caches .cargo/ and target/

hashes Cargo.lock for the key automatically

hence their removal from the ci script

I kept the cache-name around and added a target variable as well.

Is it any better?

I think the best way to figure that out would be to wait for another commit to be made to master which I can merge to my fork so we can compare the CI times between that and the original.

I forked at b36ffc9 and here's some numbers

original repo: 27m 21s
fork with invalidated cache: 25m 35s
fork with warm cache: 14m 5s

In my experience, +- 2 minutes can often just be random noise when it comes to GA

I would guess that the warm cache time is more representative of the change since, well, the original repository should also have a warm cache. But again, a more serious comparison would be to wait for the next commit.

And also not having a wall of warnings makes it easier to spot any that are actually relevant when they appear 😅

Other

I also mention this in the yml itself but for some reason there seems to be some issue with the x86_64-unknown-linux-musl target and the dtolnay/rust-toolchain action.

Even though the rustup target should be installed, the subsequent build fails. I instead install it manually which works fine (it is also cached so subsequent runs don't suffer).

Let me know if you have any questions or want any changes made!

Also the last CI run after I changed a comment decided to fail due to a timeout, I'm guessing because I've been running it quite a few times in the last few hours. Anyway, seems unrelated + i ran the failed task again and it worked fine

This reverts commit 58a95b0.

It turned out that the target was indeed being installed but it was cached across the different rust channels (1.65, stable, nightly) and I happened to not check the one which ran first, leading to my confusion.

extrawurst · 2023-08-29T13:29:48Z

can you make sure cd.yml is kept in sync?

AntoniosBarotsis · 2023-08-29T13:33:48Z

I can take a look at that sometime today/tomorrow as well

AntoniosBarotsis · 2023-08-29T13:35:09Z

I merged f639f4a and c38b1d1 into my fork and here's some more numbers:

original repo: 21m 26s and 20m 27s
fork (ci branch): 10m 43s
fork (master branch): 26m 58s

The fork's master branch was included as a sanity check to make sure that it has roughly the same times as the original repo (which it should as I have not modified it). Somehow it ended up being a bit slower, go figure.

The original repo shows 2 times as each commit was pushed separately unlike my fork where both were pushed together.

The great news is that this also improves the build task times!

Overall, this seems to be quite a bit faster, at least in these 2 commits that I tested it on.

I won't be able to really test the CD workflow but I'll make the same changes there nontheless.

extrawurst · 2023-08-29T14:28:38Z

would like to put this into action in 0.24.1 patch release tomorrow. will wait for the changes in cd.yml

AntoniosBarotsis · 2023-08-29T14:40:26Z

I'll definitely try to have it ready by then so I can see it tested as well.

AntoniosBarotsis · 2023-08-29T16:28:38Z

I checked the actions used in the CD workflow and made sure they are either on their latest version + switched the toolchain one as I did in the CI workflow earlier.

I also opted to use ${{ matrix.os }}-${{ env.cache-name }}-${{ matrix.rust }} as the cache key everywhere instead of prefixing build-linux-musl and build-linux-arm with the target. This should not change the CI times, I'll run a test on an empty commit to make sure that that is the case.

Lastly, by getting CD to use the same cache key format, it should benefit from the fresh cache that CI created in the last commit.

AntoniosBarotsis · 2023-08-29T17:03:15Z

I changed from the key attribute to shared-key to achieve this. Manually inspecting the generated keys (which you can view in the logs of each action > Restore Cargo Cache > Cache Configuration > Cache Key) reveals that for example build-linux-arm (stable) and build (ubuntu-latest, stable) have the same cache key which was not the case before. This should also guarantee that the CD workflow will use the existing cache generated by CI.

I'm gonna wait for the cache to warm up and run the workflow once more on an empty commit to make sure nothing changed in the times. After that, I think everything is done from my side so far, let me know!

AntoniosBarotsis · 2023-08-29T17:41:27Z

This actually seems to worsen performance as sometimes the cache is not properly saved since multiple jobs are trying to access it at the same time

Failed to save: Unable to reserve cache with key v0-rust-ubuntu-latest-ci-1.65-e8f5d070-722cda0f, 
another job may be creating this cache. More details: Cache already exists.

To get around this, I will only keep the shared keys for one of the CI jobs (which is really all that was needed, in retrospect). Taking a look at the latest CD run it seems that most of the time is actually spent building for windows and not arm which I would initially expect. This means that sharing the cache from the build job should be pretty effective in reducing CD times.

AntoniosBarotsis · 2023-08-29T17:56:06Z

This last change has made CI run in ~10 minutes again like last time so it seems to be working fine again.

This should also benefit CD heavily, to make sure that the cache is being utilized correctly when we do run CD we should check that the key for build (windows-latest, stable) for example is the same as release (windows-latest).

One small issue with this is that CI currently does not run on ubuntu-22.04, just ubuntu-latest which, while they are technically the same I belive, will result in different cache keys and thus the CD task won't have a cache to utilize from the previous CI run. Each CD run however will update the cache meaning that the next run will have one, just not as up to date as the other OSes.

AntoniosBarotsis · 2023-08-29T18:42:56Z

Finally, I also added caching in the udeps and linting jobs since rust-cache also caches the binaries found in ~/.cargo.

In the best case scenario, CI can take as little as 8m 19s which is over an hour of total runtime less than the existing workflow
(1h 21m 39s vs 2h 32m 54s)

Initially, I wasn't planning on adding this but it seems that with no code changes they actually take more time than all other steps.

Sorry for the influx of comments, I hadn't done some of this stuff before 😅
If something is unclear, let me know!

extrawurst · 2023-08-30T07:57:37Z

Thank you!

AntoniosBarotsis added 12 commits August 28, 2023 14:08

Use actions/checkout@v3

81fec34

Switch to dtolnay/rust-toolchain

cb1a46a

Fix typo

a205945

Try manually install target

8601e64

Add Swatinem/rust-cache

dc6a595

Add comment for target install

1237f90

Empty commit to test CI

26b9152

Invalidade cache for testing

58a95b0

Switch to rustsec/audit-check

d88cc8a

Revert "Invalidade cache for testing"

cf174da

This reverts commit 58a95b0.

Fix comment

c22a997

It turned out that the target was indeed being installed but it was cached across the different rust channels (1.65, stable, nightly) and I happened to not check the one which ran first, leading to my confusion.

Merge branch 'master' into ci

e3eb6f3

extrawurst mentioned this pull request Aug 29, 2023

Unable to switch to branches with names containing a '/' character #1851

Closed

AntoniosBarotsis added 4 commits August 29, 2023 19:19

Bump actions/checkout

8548a1a

Switch to dtolnay/rust-toolchain

2a8204e

Add cache

aa12723

Use OS instead of target in cache keys

5072f3a

AntoniosBarotsis added 2 commits August 29, 2023 19:44

Add shared key

5143123

Properly use shared keys

5578018

AntoniosBarotsis added 2 commits August 29, 2023 20:16

Empty commit to test CI

e45ba4c

Only share the build job cache key

eb9a65c

AntoniosBarotsis added 4 commits August 29, 2023 20:59

Empty commit to test CI

cadd8c9

Add cache to udeps and linting jobs

2e9ab3c

Should be key not shared-key

5a730b0

Empty commit to test CI

4839964

extrawurst merged commit ea9314e into gitui-org:master Aug 30, 2023

IndianBoy42 pushed a commit to IndianBoy42/gitui that referenced this pull request Jun 4, 2024

Speedup CI (gitui-org#1852)

2498be1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Speedup CI #1852

Speedup CI #1852

AntoniosBarotsis commented Aug 28, 2023 •

edited

Loading

extrawurst commented Aug 29, 2023

AntoniosBarotsis commented Aug 29, 2023

AntoniosBarotsis commented Aug 29, 2023

extrawurst commented Aug 29, 2023

AntoniosBarotsis commented Aug 29, 2023

AntoniosBarotsis commented Aug 29, 2023 •

edited

Loading

AntoniosBarotsis commented Aug 29, 2023

AntoniosBarotsis commented Aug 29, 2023

AntoniosBarotsis commented Aug 29, 2023 •

edited

Loading

AntoniosBarotsis commented Aug 29, 2023 •

edited

Loading

extrawurst commented Aug 30, 2023

Speedup CI #1852

Speedup CI #1852

Conversation

AntoniosBarotsis commented Aug 28, 2023 • edited Loading

Changes

Fix Warnings

Caching

Is it any better?

Other

extrawurst commented Aug 29, 2023

AntoniosBarotsis commented Aug 29, 2023

AntoniosBarotsis commented Aug 29, 2023

extrawurst commented Aug 29, 2023

AntoniosBarotsis commented Aug 29, 2023

AntoniosBarotsis commented Aug 29, 2023 • edited Loading

AntoniosBarotsis commented Aug 29, 2023

AntoniosBarotsis commented Aug 29, 2023

AntoniosBarotsis commented Aug 29, 2023 • edited Loading

AntoniosBarotsis commented Aug 29, 2023 • edited Loading

extrawurst commented Aug 30, 2023

AntoniosBarotsis commented Aug 28, 2023 •

edited

Loading

AntoniosBarotsis commented Aug 29, 2023 •

edited

Loading

AntoniosBarotsis commented Aug 29, 2023 •

edited

Loading

AntoniosBarotsis commented Aug 29, 2023 •

edited

Loading